-
Notifications
You must be signed in to change notification settings - Fork 19
Migrate discord OAuth to fastapi #50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Migrate discord OAuth to fastapi #50
Conversation
takos22
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change validation to pydantic and change some things.
takos22
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some doc changes are needed but otherwise lgtm
|
And fix the checks |
takos22
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
| @click.option("-h", "--host", default="127.0.0.1") | ||
| @click.option("-d", "--debug", default=False, is_flag=True) | ||
| @click.option("-i", "--initdb", default=False, is_flag=True) | ||
| @click.option("-r", "--reload", default=False, is_flag=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could make it a resetdb command, I've found it useful in testing from time to time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be added in another pr
|
@mohamed040406 are you planning to do the changes or should someone else do them? |
technically anyone can do them 👀 |
SylteA
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
takos22
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm except for import orders
api/app.py
Outdated
| from utils.response import JSONResponse | ||
| from aiohttp import ClientSession | ||
| from api import versions | ||
| import logging |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ordering isn't logical at all, ordering by length doesn't make sense to me. Order by first party, third party and local packages and in each of them put imports then from ... imports and each of these categories order alphabetically or just use isort.
Summary
Migrate discord OAuth from quart to fastapi
Checklist